Skip to content

refactor: create mixin to share helper functions across repos #597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
gwynndp wants to merge 5 commits into
base: master
Choose a base branch
from
Open

refactor: create mixin to share helper functions across repos #597

gwynndp wants to merge 5 commits into from

Conversation

@gwynndp
Copy link
Collaborator

@gwynndp gwynndp commented Oct 27, 2021 •
edited
Loading

Description

Organization of helper functions to reduce duplication across repositories

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines

What kind of change does this PR introduce?

  • Enhancement

Issue

What is the current behavior?

There are duplicate functions to help build queries for filtering by organization information in the Trees, Planter, and Organization repositories

What is the new behavior?

Can now use a mixin with a repository to import the helper functions from utils.repository-mixin.ts

Steps taken:

  • Created utils.repository-mixin.ts to house helper functions to integrate organization info into filter queries for use by repositories
  • Moved helper functions from Trees, Planter, & Organization repositories to utils.repository-mixin.ts to reduce duplication
  • added parameters to handle the Trees and Planters query builds differently based on different table names for the organization and id fields
  • update the controllers to pass the necessary information for building the queries
  • fix the query for when the organization has a value of null, represented as "Not Set" in the UI dropdown

Breaking change

Does this PR introduce a breaking change?

  • No

@gwynndp
Copy link
Collaborator Author

gwynndp commented Oct 27, 2021 •
edited
Loading

@nmcharlton Do you want me to write tests for the mixin or not to bother because we're moving to microservices?

I'm also not sure about the queries for "Not set" organizations. The queries filtered for null orgs and for any planters with null orgs. I tried working on this because the captures page was always getting the same result as "All" for "Not set", but the numbers aren't adding up.

@gwynndp gwynndp marked this pull request as ready for review October 30, 2021 17:00
nmcharlton
nmcharlton requested changes Nov 4, 2021
View reviewed changes
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to say don't worry about tests, but this logic has caused us a few problems.
If basic tests can be added reasonably easily, it might prevent some headaches should we need to extend the functionality.

@gwynndp gwynndp force-pushed the cleanup-helper-fns branch from 8c20ed1 to 3a56200 Compare November 7, 2021 22:47
@gwynndp gwynndp force-pushed the cleanup-helper-fns branch from 3a56200 to ec76ec9 Compare November 8, 2021 18:47
gwynndp
gwynndp commented Nov 14, 2021
View reviewed changes
src/js/buildFilterQuery.js
.replace(/"/g, '');

whereObjClause.sql = newQueryFields;

Copy link
Collaborator Author

@gwynndp gwynndp Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nmcharlton
I added this regex to add the modelName back onto each field because our more complex cross-table queries were resulting in "ambiguous" field errors that I couldn't find any other way of resolving. Is there a better way to resolve those errors?

Copy link
Collaborator

@nmcharlton nmcharlton Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether the LoopBack query builders have a way of including the table name in the query.
This looks ok, but won't catch every WHERE clause construction.

@gwynndp gwynndp force-pushed the cleanup-helper-fns branch from 4f18f74 to b016d72 Compare January 9, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nmcharlton nmcharlton nmcharlton approved these changes

@dadiorchen dadiorchen Awaiting requested review from dadiorchen dadiorchen is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gwynndp @nmcharlton